Migrate CruiseControlMetricsReporterTest to TestContainers by kyguy · Pull Request #2303 · linkedin/cruise-control

kyguy · 2025-08-11T21:54:49Z

Summary

Why: As detailed in issue Migrate off internal Kafka APIs to Improve Compatibility and Maintainability #2282, the existing Cruise Control tests rely on the Kafka server wrapper class, CCEmbeddedBroker, which uses several non-public Kafka APIs. These internal APIs lack stability and compatibility guarantees, making it increasingly difficult to upgrade Kafka versions. As Kafka evolves, this reliance introduces upgrade friction, version lock-in, and a growing maintenance burden.
What: This PR starts the migration of the Cruise Control tests away from internal Kafka APIs by refactoring the CruiseControlMetricsReporterTest to replace its dependency on the CCEmbeddedBroker class with the TestContainers Kafka module instead. This helps future-proof Cruise Control by easing upgrades to newer Kafka versions, expanding Kafka versions compatibility, and reducing the ongoing maintenance overhead.

If an approach looks good, passes review, and is merged, we can continue migrating the remaining test classes incrementally, one PR at a time, until we completely remove the dependency on private Kafka APIs in the Cruise Control tests.

Categorization

This PR partially resolves #2282

kyguy · 2025-08-21T00:40:19Z

Could one of the maintainers take a look at this PR and the referenced issue when you have a chance?
If it would be helpful, I’d be happy to tag some people in the community with Cruise Control or TestContainers experience to help with code review!

CC @mhratson @CCisGG

CCisGG · 2025-08-21T02:37:34Z

@kyguy The idea looks good to me. It would be super helpful if you could find some reviewers, and I'll give a +1 once this get reviewed. Thanks!

kyguy · 2025-08-21T11:45:01Z

Hoping to tap into your expertise in Cruise Control and/or TestContainers, would you mind taking a look at this when you get a chance?

CC @ppatierno @fvaleri @ShubhamRwt @tomncooper @mimaison @see-quick @k-wall

see-quick

Looks good to me 👍. I have just a few comments to consider.

see-quick

LGTM 👍

mimaison

Thanks for the PR! I left a few suggestions and comments

mimaison

Thanks for the updates. I made another pass

mimaison

Thanks for the updates! It's looking better, I just left a few more comments.

mimaison · 2025-09-09T10:09:54Z

Thanks for the updates.

It looks like the latest changes have broken a test:

com.linkedin.kafka.cruisecontrol.TopicAnomalyIntegrationTest > testTopicAnomalyFinder[0] FAILED
    java.lang.NullPointerException
        at com.linkedin.kafka.cruisecontrol.KafkaCruiseControlVertxApp.serverUrl(KafkaCruiseControlVertxApp.java:31)
        at com.linkedin.kafka.cruisecontrol.KafkaCruiseControlApp.printStartupInfo(KafkaCruiseControlApp.java:79)
        at com.linkedin.kafka.cruisecontrol.KafkaCruiseControlVertxApp.start(KafkaCruiseControlVertxApp.java:58)
        at com.linkedin.kafka.cruisecontrol.CruiseControlIntegrationTestHarness.start(CruiseControlIntegrationTestHarness.java:47)
        at com.linkedin.kafka.cruisecontrol.TopicAnomalyIntegrationTest.setup(TopicAnomalyIntegrationTest.java:62)

kyguy · 2025-09-11T14:19:58Z

Ready for another round of reviews, @k-wall @mimaison could you have another pass when you get a chance?

mimaison

LGTM

k-wall

One final nit, but otherwise LGTM

kyguy · 2025-09-12T21:07:41Z

Hey @CCisGG this PR has been tested, reviewed, and is ready to go!

Signed-off-by: Kyle Liberti <kliberti.us@gmail.com>

kyguy · 2025-10-01T17:17:29Z

Hi @CCisGG, if you don’t have any objections, would you be okay with merging this PR?

danielgospodinow

Took a shallow first look at the PR and left some comments. I'll revisit the PR soon to see if I can find anything deeper.

Btw, this is a fantastic initiative, IMO. Kudos for the effort!

danielgospodinow · 2025-10-01T20:36:31Z

+  // Selects or creates the Docker network used for Test Container communication.
+  // This is useful in CI environment like CircleCI where the Docker network needs to be shared with Test Container client.
+  private static final Network NETWORK = System.getenv("NETWORK_NAME") != null
+    ? Network.builder().id(System.getenv("NETWORK_NAME")).build()


FWIW, setting the network ID using id(name) seems to be deprecated. 🤔

Yes, id(name) has been deprecated but it is required for connecting CircleCI jobs to TestContainer services. This is something I need to chase up with the TestContainers and CircleCI communities so we don't lose this functionality in the future or are presented an alternative path forward. I'll link an issue here.

danielgospodinow · 2025-10-01T20:41:47Z

          no_output_timeout: 15m
-          command: ./gradlew --no-daemon -PmaxParallelForks=1 build
+          command: |
+            export NETWORK_NAME="test_containers_network"


Is it mandatory to create and pass down NETWORK_NAME and CONTAINER_HOST?

If everything works fine without those two (in case the framework creates its own ephemeral resources), I'd personally prefer not having them for the sake of simplicity.

Reading the comments in CCContainerizedKraftCluster kind of explained the situation. It's basically good to have for CI. But I'm still not sure if it's mandatory or just good to have.

Unfortunately, these variables are mandatory for allowing the CircleCI job to find/connect to the containers created by TestContainers. I have just added some comment/Javadocs to make this more clear. Let me know what you think!

Got it. Makes sense. And the docs explain it well now. Thanks!

danielgospodinow · 2025-10-01T20:59:23Z

+      EXTERNAL_LISTENER_NAME + "://0.0.0.0:" + CONTAINER_EXTERNAL_LISTENER_PORT
+    );
+    this._bootstrapServers = generateBootstrapServersList(numOfBrokers, containerHostPorts);
+    adminClientProps.setProperty(CommonClientConfigs.BOOTSTRAP_SERVERS_CONFIG, _bootstrapServers);


It's slightly odd that we're mutating an externally provided object that is only used here. It can cause side effects. Why not just craft the adminClientProps here in the constructor?

Or maybe we can pass a clone of adminClientProps.

Or just have JavaDoc that mentions this behavior clearly.

Cloned it in the constructor and added a note in the latest commit, let me know if this will suffice!

Signed-off-by: Kyle Liberti <kliberti.us@gmail.com>

CCisGG

Thanks @kyguy !

…2303) * Migrate CruiseControlMetricsReporterTest to TestContainers Signed-off-by: Kyle Liberti <kliberti.us@gmail.com> * Addressing feedback - gp Signed-off-by: Kyle Liberti <kliberti.us@gmail.com> --------- Signed-off-by: Kyle Liberti <kliberti.us@gmail.com>

kyguy force-pushed the test-containers-migration branch 6 times, most recently from dbf97d8 to e0b2cb3 Compare August 20, 2025 20:57

see-quick reviewed Aug 22, 2025

View reviewed changes